Repository navigation
Core: Make PlanTableScanResponse buider withCredentials replace instead of append - #17751
Conversation
dramaticlly
changed the title
|
@singhpk234 @nastra @huaxingao if you want to take a look |
| credentials.addAll(credentialsToAdd); | ||
| public Builder withCredentials(List<Credential> newCredentials) { | ||
| Preconditions.checkArgument(null != newCredentials, "Invalid credentials list: null"); | ||
| Preconditions.checkArgument(!newCredentials.contains(null), "Invalid credential: null"); |
There was a problem hiding this comment.
do we actually need this check? We should get this for free when doing this.credentials = ImmutableList.copyOf(credentialsToAdd)
There was a problem hiding this comment.
I think ImmutableList.copyOf(credentialsToAdd) will throw NPE instead of IllegalArgumentException instead, from AGENTS.md seem to favor "Preconditions.checkArgument over NPE"
and I saw we have some precedence in
| public Builder withCredentials(List<Credential> credentialsToAdd) { | ||
| credentials.addAll(credentialsToAdd); | ||
| public Builder withCredentials(List<Credential> newCredentials) { | ||
| Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null"); |
There was a problem hiding this comment.
per
| Preconditions.checkArgument(null != newCredentials, "Invalid credentials: null"); | |
| Preconditions.checkArgument(newCredentials, "Invalid credentials list : null"); |
There was a problem hiding this comment.
Thanks @singhpk234 , I think ListTableResponse is relative old (merged Feb 2022) compare to the latest agents.md guideline in
Line 121 in f80a72f
1ed9d07 to
9f2604d
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
@singhpk234 @nastra if you want to take another look at the PR, or we can let it to be closed by stale bot |
|
thanks @dramaticlly for fixing this. Let's wait a few more days before merging in case @singhpk234 has any further comments |
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM too, thanks @dramaticlly !
|
Thanks @nastra for the review ! |
3 participants
Follow up of #17638 (comment) with changes in
PlanTableScanResponse.BuilderandFetchPlanningResultResponse.BuilderwithCredentials(List<Credential>)now replaces the builder's credentials instead of appending to them.nulllist andnullelements as the follow up of Core: Add storage credentials to FetchPlanningResultResponse #14994 (comment)build()now passesImmutableList.copyOf(credentials)instead of the builder's liveArrayList.AI Disclosure
withCredentialsappend-vs-replace question raised in review of Core: Reduce visibility on scan planning response builder for specsById and deleteFiles #17638.